Fix vendored revert leaving created scaffold behind (#636, #670) - #672
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Conversation
Assisted-by: Claude Code:claude-opus-5-5
When two or more packages were vendored into a pnpm or uv project, only the first ledger entry remembered that vendoring created the shared pnpm.overrides / pnpm table, pnpm-workspace.yaml or [tool.uv.sources] table. A full vendor --revert, rollback or remove then left that scaffolding behind unless the first package happened to be reverted last, leaving the checkout dirty. The ledger now shares those "created by vendoring" flags across every entry that wires the same files, when it is written and when it is read, so whichever entry empties the scaffold removes it. Ledgers written by earlier releases are repaired on read. Scaffolding that still holds anything else, such as user keys, is kept as before. Fixes #636, #670 Assisted-by: Claude Code:claude-opus-5-5
58ef733 to
d46b5fa
Compare
Vendors two packages into a pnpm 9.0-lock project with no pnpm table and no workspace file, then runs vendor --revert end to end, checking package.json, the lock and pnpm-workspace.yaml all come back exact (#636). Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
The pnpm legacy-ledger fixture recorded what the earlier binary left after reverting two vendored packages: an empty pnpm.overrides in package.json and the scaffolded pnpm-workspace.yaml (#636). Reverting that same ledger now removes both, so the golden is the restored package.json (still re-indented, as before) and no workspace file. Assisted-by: Claude Code:claude-opus-5-5
The group-commit round-trip test exempted pnpm from the byte-exact check because a two-package revert left the emptied pnpm.overrides and the scaffolded pnpm-workspace.yaml behind (#636). Now that both are removed, say why pnpm is still exempt (the fixture's minified package.json comes back re-indented) and assert the scaffold is gone. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0167c0b. Configure here.
|
[agent] I don't think this PR caused it:
The FAIL row has an empty error field. The real reason is in Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
|
[burn-down agent] Labeled Ready for review at head
Generated by Claude Code |
|
Codex review of The shared creation flags are normalized on ordinary and group-commit ledger reads and writes. This fixes creator-first cleanup for pnpm and uv while preserving the existing checks that retain user-owned or changed configuration. The production delta is confined to the ledger implementation; the pnpm/uv revert backends are unchanged. Validation on this exact commit:
Current remote checks: 476 successful checks, 7 skipped; 11 successful workflows, 1 skipped. The latest Bun replacement jobs and current PDM run were verified. Bugbot is clean on this commit, with no unresolved review threads or new actionable feedback. |
This branch has not been deployed
2 participants
LLM Description written by Claude Code:claude-opus-5-5
Fixes #636
Fixes #670
Summary
After two or more packages are vendored into a pnpm or uv project, a full
vendor --revert, arollback, or aremovesequence now removes the scaffolding vendoring created. Before, depending on revert order, it could leave an empty"pnpm": { "overrides": {} }, a scaffoldedpnpm-workspace.yaml/ emptyoverrides:section, or an empty[tool.uv.sources]header behind.Root cause
When vendored mode wires two or more packages into a project, the first one creates any shared scaffolding: the pnpm
package.jsonpnpm/pnpm.overridestables, apnpm-workspace.yaml(or itsoverrides:section), or uv's[tool.uv.sources]table. Whether vendoring created that scaffolding is recorded only on the first package's ledger entry (PnpmMeta.created_*,UvMeta.created_sources_table). Each later package finds the scaffolding already there and recordsfalse.Revert removes an emptied scaffold only when the entry being reverted carries the flag. So cleanup happened only when the creator entry was reverted last.
vendor --revertgoes in purl order, which reverts the creator first in both reported fixtures.Fix
"Vendor created this scaffold" is now a property of the shared files rather than of one ledger entry.
VendorState::share_scaffold_flagstakes the union of the flags across every entry that wires the same project-root files: all pnpm entries sharepackage.json/pnpm-workspace.yaml, and all uv entries sharepyproject.toml. It is applied:ledger_value, covering bothsave_stateand the group-commit render), so new ledgers persist the shared flags;parse_state, plus both group-commit value paths), so ledgers written by earlier releases are repaired, in memory and on their next save. A no-op re-vendor still writes nothing.Whichever entry empties the scaffold now removes it, in any order. Revert's existing emptiness and scaffold-match checks are unchanged, so scaffolding holding anything else is kept: user keys in
pnpm, a user-authored[tool.uv.sources], or a workspace file that has drifted. This uses the same OR-merge reasoningcarry_forward_wiringalready applies to a same-package re-vendor. The fix lives in one place in the ledger, and no backend revert code changed. Thenpm/,pypi/andgem/wrappers only dispatch to the binary, so they need no change.Goldens that recorded the bug
tests/fixtures/legacy-ledgers/pnpm/reverted/recorded the earlier binary's two-package revert, residue included. The fixture now holds the restoredpackage.jsonwith nopnpmtable, and nopnpm-workspace.yaml. pnpm stays out of that suite's byte-exact assertion only because the fixture's minifiedpackage.jsoncomes back re-indented, which is a separate limitation that predates this PR.vendor_group_commit_e2e: its comment called the residue expected behavior. It now asserts pnpm leaves no workspace file and nopnpmtable.Tests (red → green)
The core tests vendor two packages through the real backend, persist the ledger after each one as the vendor loop does, then revert one entry at a time from a freshly loaded ledger, saving after each removal. That is the shape of
vendor --revert,rollbackand successiveremoveruns. The CLI test runs the real binary end to end.e2e_vendor_pnpm_build::pnpm9_two_packages_vendor_revert_removes_created_scaffold(realvendor/vendor --revert; lock matches what real pnpm 10.28 generated)"pnpm": {"overrides": {}}leftpnpm_lock::tests::revert_two_packages_creator_first_removes_created_scaffoldpnpm_lock::tests::revert_two_packages_removes_created_workspace_overridesoverrides:leftpnpm_lock::tests::revert_two_packages_repairs_a_creator_only_ledgerpnpm_lock::tests::revert_two_packages_keeps_a_user_pnpm_tablepnpm_lock::tests::revert_two_packages_creator_last_removes_created_scaffold(control)vendor_ledger_schema_e2e(legacy pnpm ledger),vendor_group_commit_e2epypi_uv::tests::revert_two_packages_creator_first_removes_created_sources_table[tool.uv.sources]leftpypi_uv::tests::revert_two_packages_repairs_a_creator_only_ledgerpypi_uv::tests::revert_two_packages_keeps_a_user_sources_tablepypi_uv::tests::revert_two_packages_creator_last_removes_created_sources_table(control)state::tests::created_scaffold_flags_are_shared_across_entriesThe #670 coverage is at the backend level: real
wire_uv/revert_uvplus the ledger round trip. Producing a two-package uv CLI fixture would need the patch service to build two wheels.Local checks
cargo clippy --workspace --all-features -- -D warnings: clean.cargo test -p socket-patch-core --all-features --lib: 4852 passed. 4 failed, all permission-mode tests (chmod 0o555/0o444) that can't fail as root in this sandbox (copy_tree,vlt_heal,pypi_poetry,pypi_requirements). None of them touches this change.cargo test -p socket-patch-cli --all-features --lib: 834 passed.--test e2e_vendor_pnpm_build: 16 passed.--test e2e_vendor_pypi_build(real uv 0.8.17): 17 passed.--test mode_migration_pypi: 12 passed.--test vendor_ledger_schema_e2e: 3 passed.--test vendor_group_commit_e2e: 7 passed.--test covgap_commands_vendor: 41 passed. 3*_state_write_failure_*tests failed because they rely on a read-only.socket/vendordir, which root bypasses (same environment limit as above).node --test npm/socket-patch/bin/socket-patch.test.mjs: 4 passed.cargo test --workspacecouldn't finish locally because the sandbox ran out of disk while linking test binaries. CI runs the full matrix.cargo fmt:mainitself isn't rustfmt-clean (CI has no fmt gate). Every file this PR touches was clean onmainand is still clean.CI notes
On
8cd72d7,test (ubuntu-latest)andcoveragefailed on the outdated pnpm golden above, fixed in8212cc8.native (ubuntu-latest, 2.29.2)(PDM backtest) failed one agent-mode cell (marker/appliedExactlyOne). Agent mode never reads the vendor ledger. That cell passes on main and on other open PRs, and it passed on the re-run on0167c0b.On
0167c0b,binary (macos-latest)(Bun lockb backtest) failed 1 of 25 cells (0.5.9 writer=0.1.6). Bun entries carry no pnpm/uv metadata, so this change can't affect them. The same job passed on8cd72d7, which already contained the fix, and it passes on main. It passed on re-run (job 111191290748), so all CI is green on0167c0b.🤖 Generated with Claude Code
https://claude.ai/code/session_013yeY9QVvU6wJNVtWQi5MjY
Generated by Claude Code